[FEATURE] Data,Refinery: add QR code support#11391
[FEATURE] Data,Refinery: add QR code support#11391thibsy wants to merge 3 commits intoILIAS-eLearning:trunkfrom
Conversation
613b2fe to
3de0fa2
Compare
* Add `ILIAS\Data\QR\SVGCode` wrapper for generated QR codes. * Add `ILIAS\Data\QR\ErrorCorrectionLevel` for standardised error correction levels. * Add `ILIAS\Refinery\String\QRCode` transformation to convert string into a QR code. * Add unit tests covering the functionality above.
3de0fa2 to
0e88c35
Compare
|
Assessment: The General Information:
Type of dependency:
Usage:
Reasoning:
Maintenance:
Links:
Alternatives: There is only one other widely used library, which does not rely on the
|
|
Hi @thibsy, here are my thoughts on this: In terms of accessibility: Kind regards and thank you again for this PR |
|
Hi @lscharmer, thx for the quick feedback! I think its not quite possible to pass around the information which makes up the QR-code, since this reaches into the dependency too far. We could only introduce a wrapper so we can limit what kind of data can be transformed into a QR-code result, but this wouldn't add much value. I'm therefore beginning to think we should just drop the DTO and only work in transformations. How does that sound to you? Give me a quick ping if we should discuss this on Discord. Also thanks for your thoughts on the accessibility. You're totally right – this is exactly what my UI interface is starting to look like (encapsulate different formats of which QR code is only one). Kind regards, |
|
Hi @lscharmer, As quickly discussed on Discord, I have removed the QR code DTO for the reasons above and implemented the abstraction primarily based on transformations. This PR now introduces:
Since the Looking forward to a more concrete review now =). Thx and kind regards, |
|
P.S. once the changes are approved I will squash the commits appropriately, so we can rebase and integrate one commit for the abstraction (or two for each component if you prefer) and one for the dependency. |
lscharmer
left a comment
There was a problem hiding this comment.
Hi @thibsy,
thanks updating this PR. The structure looks good to me, there is just one naming issue left.
Best regards
@lscharmer
| return new Transformation\ToStringTransformation(); | ||
| } | ||
|
|
||
| public function toSvg( |
There was a problem hiding this comment.
The name toSvg says nothing about a QR code (same with the corresponding ToSvgTransformation class).
Please rename the method and transformation (maybe something like toSvgQRCode or so).
| namespace ILIAS\Refinery\URI; | ||
|
|
||
| use ILIAS\Refinery\Transformation; | ||
| use ILIAS\Refinery\Transformation as ITransformation; |
There was a problem hiding this comment.
| use ILIAS\Refinery\Transformation as ITransformation; | |
| use ILIAS\Refinery\Transformation; |
| use ILIAS\Refinery\Constraint; | ||
| use ILIAS\Refinery\Transformation; | ||
| use ILIAS\Refinery\String\Encoding\Group as EncodingGroup; | ||
| use ILIAS\Data\QR\ErrorCorrectionLevel; |
There was a problem hiding this comment.
| use ILIAS\Data\QR\ErrorCorrectionLevel; |
Hi @mjansenDatabay,
This is the first iteration of my QR code abstraction for ILIAS, which I plan on using inside the UI framework to convert
ILIAS\Data\URIobjects into a QR code which can be embedded inline as a data URI.I haven't worked out the exact details of the UI interface yet, so there may be changes in the meantime, but I'd still like to hear your 5 cents about this – it doesn't need to be an in-depth code review just yet. I will also wait with the JF label until you have approved of the details, although I will post the formal request in here some time soon already.
What I have worked out:
What I haven't worked out (yet):
Looking forward to your thoughts.
Thx and kind regards,
@thibsy